Skip to content

fix(local_runtime): Improve local_runtime usability in macos / windows #3148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

laramiel
Copy link

@laramiel laramiel commented Aug 6, 2025

local_runtime fails to handle many variations of python install on windows and MacOS, such as:

  • LDLIBRARY on MacOS may refer to a file under PYTHONFRAMEWORKPREFIX, not LIBDIR
  • LDLIBRARY on Windows refers to pythonXY.dll, not the linkable pythonXY.lib
  • LIBDIR may not be correctly set on Windows.
  • On windows, interpreter_path needs to be normalized. Other paths also require this.
  • SHLIB_SUFFIX does not indicate the correct suffix.

For examples, see

See: https://docs.python.org/3/extending/windows.html

In order to resolve this the shared library resolution has been moved into get_local_runtime_info.py, which now does the following:

  • Constructs a list of paths to search based on LIBDIR, LIBPL, PYTHONFRAMEWORKPREFIX, and the executable directory.
  • Constructs a list of libraries to search based on INSTSONAME, LDLIBRARY, pythonXY.lib, etc.
  • Checks to see which files exist, partitioning the result into a list of "dynamic_libraries" and "static_libraries"

On windows and macos, since SHLIB_SUFFIX does not always indicate the filenames needed searching, this has been removed from local_runtime_repo_setup and replaced with an explicit file.

On windows the interpreter_path and other search paths are now normalized ( \ converted to /).

Additional logging added to local_runtime_repo.

Fixes #3055
Work towards #824

local_runtime fails to handle many variations of python install on windows and MacOS, such as:
* LDLIBRARY on MacOS may refer to a file under PYTHONFRAMEWORKPREFIX, not LIBDIR
* LDLIBRARY on Windows refers to pythonXY.dll, not the linkable pythonXY.lib
* LIBDIR may not be correctly set on Windows.
* On windows, interpreter_path needs to be normalized.  Other paths also require this.
* SHLIB_SUFFIX does not indicate the correct suffix.

For examples, see
See: bazel-contrib#3055
See: https://docs.python.org/3/extending/windows.html

Example get_local_runtime_info.py outputs:

rules_python:local_runtime_repo(@@local_python3) INFO: GetPythonInfo result:
  INSTSONAME: Python.framework/Versions/3.12/Python
  LDLIBRARY: Python.framework/Versions/3.12/Python
  LIBDIR: /Library/Frameworks/Python.framework/Versions/3.12/lib
  MULTIARCH: darwin
  PY3LIBRARY: <empty string>
  PYTHONFRAMEWORKPREFIX: /Library/Frameworks
  base_executable: /Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12
  implementation_name: cpython
  include: /Library/Frameworks/Python.framework/Versions/3.12/include/python3.12
  major: 3
  micro: 10
  minor: 12

rules_python:local_runtime_repo(@@local_python3)  INFO: GetPythonInfo result:
  INSTSONAME: None
  LDLIBRARY: python313.dll
  LIBDIR: T:\build_temp\home\python_4379bdec28bdff81a567a01c9b8cf10e3856c8c966e4fe53945bedea6338b416\tools\libs
  MULTIARCH: None
  PY3LIBRARY: python313.lib
  base_executable: T:\build_temp\home\python_4379bdec28bdff81a567a01c9b8cf10e3856c8c966e4fe53945bedea6338b416\tools\python.exe
  implementation_name: cpython
  include: T:\build_temp\home\python_4379bdec28bdff81a567a01c9b8cf10e3856c8c966e4fe53945bedea6338b416\tools\Include
  major: 3
  micro: 1
  minor: 13

On windows and macos, since SHLIB_SUFFIX does not always indicate the filenames needed searching, this has been removed from local_runtime_repo_setup and replaced with an explicit list of files.  In addition, target libraries are searched in multiple locations to handle variations in macos and windows file locations.

On windows the interpreter_path and other search paths are now normalized ( \ converted to /).

On windows the get_local_runtime_info now returns the pythonXY.lib library

Additional logging added to local_runtime_repo.
:1
@laramiel
Copy link
Author

laramiel commented Aug 7, 2025

After integrating this some more with tensorstore, I've reworked the library lookup to handle manylinux and other linux installs with no dynamic libraries exposed.


rickeylev edit to preserve some initial PR info:

Example get_local_runtime_info.py outputs:

rules_python:local_runtime_repo(@@local_python3) INFO: GetPythonInfo result:
  INSTSONAME: Python.framework/Versions/3.12/Python
  LDLIBRARY: Python.framework/Versions/3.12/Python
  LIBDIR: /Library/Frameworks/Python.framework/Versions/3.12/lib
  MULTIARCH: darwin
  PY3LIBRARY: <empty string>
  PYTHONFRAMEWORKPREFIX: /Library/Frameworks
  base_executable: /Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12
  implementation_name: cpython
  include: /Library/Frameworks/Python.framework/Versions/3.12/include/python3.12
  major: 3
  micro: 10
  minor: 12

Here we see the MacOS framework issue, namely LDLIBRARY is not under LIBDIR, but directly under frameworks.
The new code adds PYTHONFRAMEWORKPREFIX to the search path, so the library is found.

rules_python:local_runtime_repo(@@local_python3)  INFO: GetPythonInfo result:
  INSTSONAME: None
  LDLIBRARY: python313.dll
  LIBDIR: T:\build_temp\home\python_4379bdec28bdff81a567a01c9b8cf10e3856c8c966e4fe53945bedea6338b416\tools\libs
  MULTIARCH: None
  PY3LIBRARY: python313.lib
  base_executable: T:\build_temp\home\python_4379bdec28bdff81a567a01c9b8cf10e3856c8c966e4fe53945bedea6338b416\tools\python.exe
  implementation_name: cpython
  include: T:\build_temp\home\python_4379bdec28bdff81a567a01c9b8cf10e3856c8c966e4fe53945bedea6338b416\tools\Include
  major: 3
  micro: 1
  minor: 13

Here we see that under windows the python313.dll is reported; python313.lib is added via PY3LIBRARY.

Three more notes on this CL; they may be areas for future improvements.

  • On MacOS, when linking against the system python, we may want to use "-Wl,-framework,Python". However this CL does not detect / change those linker options.

  • It would be nice to have tests for a minimal C python extension and minimal C python embedded use for the local_runtime as well as the hermetic runtimes; this is similar to tests/cc/current_cc_py_libs

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider also referencing the issue in the changelog.

This PR is very exciting, but I gave it a quick glance only since it is reasonably big.

I would love to ask the local_python example also be made to work with bzlmod - all of the improvements should work there too.

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oddly, I can edit and commit files via the ui, but can't push to laramiel/main on the command line. Is there something special setup with your repo or branch? I was going to move the example over into local toolchains, but it was a bit hard to refactor that via the ui alone.

@laramiel
Copy link
Author

Oddly, I can edit and commit files via the ui, but can't push to laramiel/main on the command line. Is there something special setup with your repo or branch? I was going to move the example over into local toolchains, but it was a bit hard to refactor that via the ui alone.

I wouldn't expect so; I forked using the web ui and haven't changed anything.

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly LGTM, just need to remove the example as shown and update tests/integration/local_toolchains instead.

@rickeylev
Copy link
Collaborator

Oddly, I can edit and commit files via the ui, but can't push to laramiel/main on the command line. Is there something special setup with your repo or branch? I was going to move the example over into local toolchains, but it was a bit hard to refactor that via the ui alone.

I wouldn't expect so; I forked using the web ui and haven't changed anything.

Weird 🤷 . Maybe because your fork's branch is the main branch? sometimes those have additional protections.

@rickeylev
Copy link
Collaborator

Looks like we have enough slots to run the CI jobs, but they had a lot of unrelated failures. On the plus side, I think they exercised enough of the new local toolchain logic to give some basic validation of the new code.

I don't think we should block this PR on making all of those integration tests pass. I'll reconfig the mac and windows ones to just run the local toolchain test; that looks relatively easy and feasible.

re: adding a py extension test: +1 on such a test (we have need of that elsewhere for testing), but I'm also OK with omitting it for this PR. Test-supporting code goes in tests/support

@jbms
Copy link

jbms commented Aug 12, 2025

I guess py_extension is being added for a test now but might later be made into a public rule?

For fvisibility=hidden, really that needs to propagate to all transitive dependencies instead, so should be handled via a transition or just by a global --copt option specified by the user.

@laramiel
Copy link
Author

I guess py_extension is being added for a test now but might later be made into a public rule?

It's added for test purposes, I'll leave it up to @rickeylev or others to make that determination.
It would be nice to have a common py_extension, IMO.

For fvisibility=hidden, really that needs to propagate to all transitive dependencies instead, so should be handled via a transition or just by a global --copt option specified by the user.

For now I've removed the cc_library from py_extension. That seems a little cleaner?

@rickeylev
Copy link
Collaborator

I guess py_extension is being added for a test now but might later be made into a public rule?

Maybe, but not without some more consideration and evaluation. (e.g. My current day-job team developed their own variation of py_extension, my last team created another one, and I've seen others out there, all with pretty different implementations). #824 is the FR for this (long with lots of details and cases). Your mention of how deps are handled and fvisibility are good examples of the sort of considerations to not forget. I don't want perfect to be the enemy of the good, but also want thoughtfulness put into a public feature. (All that plus I want to keep the PR scope reasonable)

copybara-service bot pushed a commit to google/tensorstore that referenced this pull request Aug 13, 2025
Replace third_party/python/python_configure with bazel/repo_rules/local_python_runtime,
which creates a local python runtime/toolchain repository
equivalent to @rules_python/python/local_toolchains:local_runtime_repo

Reference to the python headers now use the toolchain-based indirection
rather than referencing the local repository headers directly.

Includes local changes to also address:
bazel-contrib/rules_python#3055

See external version at:
bazel-contrib/rules_python#3148

PiperOrigin-RevId: 794407197
Change-Id: I210f776470bad045a031fffb0cf1f333114d8548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: Local Toolchain on Windows: local_runtime_repo generates interpreter_path without escaping backward slashes
4 participants